docs/Intune_Configuration_Guide.md (1)

194-194: Consider hyphenating "Top-Level" as a compound adjective.

Minor style suggestion: "Top Level Settings" should be "Top-Level Settings" for grammatical correctness when used as a compound modifier.

✏️ Proposed fix

-### Top Level Settings
+### Top-Level Settings

scripts/build_release.ps1 (2)

332-352: Expanded Inno Setup discovery is good, but contains redundant paths.

The path array includes both hardcoded paths (e.g., "C:\Program Files (x86)\Inno Setup 6\ISCC.exe") and environment variable paths (e.g., "${env:ProgramFiles(x86)}\Inno Setup 6\ISCC.exe") that typically resolve to the same location. This isn't a functional issue—just minor redundancy that adds a few extra Test-Path calls.

♻️ Optional: Remove redundant hardcoded paths

         $PossiblePaths = @(
-            "C:\Program Files (x86)\Inno Setup 6\ISCC.exe",
-            "C:\Program Files\Inno Setup 6\ISCC.exe",
-            "C:\Program Files (x86)\Inno Setup 5\ISCC.exe",
-            "C:\Program Files\Inno Setup 5\ISCC.exe",
             "${env:ProgramFiles(x86)}\Inno Setup 6\ISCC.exe",
             "$env:ProgramFiles\Inno Setup 6\ISCC.exe",
             "${env:ProgramFiles(x86)}\Inno Setup 5\ISCC.exe",
-            "$env:ProgramFiles\Inno Setup 5\ISCC.exe"
+            "$env:ProgramFiles\Inno Setup 5\ISCC.exe",
+            # Fallback for edge cases where env vars aren't set
+            "C:\Program Files (x86)\Inno Setup 6\ISCC.exe",
+            "C:\Program Files\Inno Setup 6\ISCC.exe"
         )

---

406-422: Duplicated Inno Setup discovery logic.

This path discovery block is identical to lines 332-349. Consider extracting this into a helper function to reduce duplication and ensure consistent behavior (e.g., the "Found Inno Setup at" message is missing here).

♻️ Extract helper function for Inno Setup discovery

Add this helper function near the other helper functions (around line 205):

function Find-InnoSetup {
    $IsccPath = (Get-Command "iscc" -ErrorAction SilentlyContinue).Source
    if ($IsccPath) {
        Write-Host "Found Inno Setup in PATH: $IsccPath" -ForegroundColor Gray
        return $IsccPath
    }
    
    $PossiblePaths = @(
        "${env:ProgramFiles(x86)}\Inno Setup 6\ISCC.exe",
        "$env:ProgramFiles\Inno Setup 6\ISCC.exe",
        "${env:ProgramFiles(x86)}\Inno Setup 5\ISCC.exe",
        "$env:ProgramFiles\Inno Setup 5\ISCC.exe"
    )
    foreach ($p in $PossiblePaths) {
        if (Test-Path $p) {
            Write-Host "Found Inno Setup at: $p" -ForegroundColor Gray
            return $p
        }
    }
    return $null
}

Then replace both discovery blocks with:

$IsccPath = Find-InnoSetup

.github/workflows/release.yml (1)

41-50: Good defense-in-depth version validation.

The shell-level validation complements the Python script's built-in fallback, providing an extra safety net. The regex correctly validates the basic MAJOR.MINOR.PATCH format.

Note: The hardcoded fallback "2026.1.2" here must be kept in sync with FALLBACK_VERSION in generate_release_version.py. Consider defining this as a workflow-level environment variable to ensure consistency.

♻️ Optional: Define fallback as workflow-level env var

 on:
   workflow_dispatch:
     inputs:
       release_type:
         # ...
 
+env:
+  FALLBACK_VERSION: "2026.1.2"
+
 permissions:
   contents: write

Then reference as ${{ env.FALLBACK_VERSION }} in the validation step.

src/switchcraft/services/intune_service.py (1)

394-403: Remove the duplicate RequestException handler.
The second block is unreachable and leaves conflicting error semantics.

♻️ Suggested cleanup

         except requests.exceptions.RequestException as e:
             logger.error(f"Network error in list_apps: {e}")
             raise
-        except requests.exceptions.RequestException as e:
-            logger.error(f"Network error while listing apps: {e}")
-            raise Exception(f"Network error: {str(e)}")

tests/test_github_login.py (2)

61-64: Remove unused get_page function definitions.

These get_page functions are defined but never used—only the property assignment on line 64/102/146 is effective. The function definitions are dead code.

♻️ Suggested cleanup

-    # Mock the page property to avoid RuntimeError
-    def get_page():
-        return mock_page
     type(view).page = property(lambda self: mock_page)

Also applies to: 99-102, 143-146

---

72-73: Consider using pytest-timeout or event-based synchronization instead of time.sleep.

Using fixed time.sleep(0.5) calls for async synchronization can lead to flaky tests—too short may cause failures on slow CI, too long wastes time. Consider using threading Events or polling with shorter sleeps and a max retry count for more reliable tests.

Also applies to: 116-117, 167-168

src/switchcraft/gui_modern/app.py (2)

188-205: Retry logic may cause redundant operations.

The retry path at lines 195-197 calls _open_notifications_drawer(e) again after it already failed in the try block (line 189). If the failure is deterministic (e.g., missing permissions, invalid state), retrying immediately won't help and wastes resources. Consider adding a different recovery strategy or removing the redundant retry.

♻️ Suggested simplification

             try:
                 self._open_notifications_drawer(e)
                 # Force update to ensure drawer is visible
                 self.page.update()
             except Exception as ex:
                 logger.exception(f"Error opening notification drawer: {ex}")
-                # Try again with simpler approach
-                try:
-                    self._open_notifications_drawer(e)
-                    self.page.update()
-                except Exception as ex2:
-                    logger.error(f"Failed to open drawer after retry: {ex2}")
-                    self.page.snack_bar = ft.SnackBar(
-                        content=ft.Text(f"Failed to open notifications: {ex2}"),
-                        bgcolor="RED"
-                    )
-                    self.page.snack_bar.open = True
-                    self.page.update()
+                self.page.snack_bar = ft.SnackBar(
+                    content=ft.Text(f"Failed to open notifications: {ex}"),
+                    bgcolor="RED"
+                )
+                self.page.snack_bar.open = True
+                self.page.update()

---

1006-1009: Triple page.update() calls are unusual.

Calling page.update() three times consecutively suggests potential underlying rendering issues. While this might work around a Flet quirk, it's a code smell. Consider investigating the root cause or adding a comment explaining why this is necessary.

tests/test_intune_store_search.py (1)

19-21: Remove unused get_page function.

The get_page function is defined but never called—only the property assignment matters.

♻️ Suggested cleanup

-    # Mock page property to avoid RuntimeError
-    def get_page():
-        return page
     type(page).page = property(lambda self: page)

tests/test_winget_details.py (1)

19-21: Remove unused get_page function.

Same issue as in other test files—the function is defined but never called.

♻️ Suggested cleanup

-    # Mock page property to avoid RuntimeError
-    def get_page():
-        return page
     type(page).page = property(lambda self: page)

tests/test_all_buttons.py (3)

41-46: Mock page is missing run_task and other attributes needed by some views.

Compared to other test fixtures in the codebase (e.g., test_button_functionality.py lines 12-28), this mock is missing run_task, dialog, end_drawer, snack_bar, open, and close. Some views like ModernIntuneStoreView and ModernWingetView may fail or behave unexpectedly during instantiation.

Suggested enhancement

     mock_page = MagicMock(spec=ft.Page)
     mock_page.update = MagicMock()
     mock_page.switchcraft_app = MagicMock()
     mock_page.switchcraft_app.goto_tab = MagicMock()
     mock_page.switchcraft_app._current_tab_index = 0
+    mock_page.switchcraft_app._view_cache = {}
+    mock_page.dialog = None
+    mock_page.end_drawer = None
+    mock_page.snack_bar = None
+    mock_page.open = MagicMock()
+    mock_page.close = MagicMock()
+    mock_page.run_task = lambda func: func()  # Execute immediately for testing
     type(mock_page).page = property(lambda self: mock_page)

---

143-145: Same test coupling issue as above.

test_button_handlers_are_callable also calls test_all_views_have_buttons() directly, creating the same fragile dependency.

---

30-112: Test function acts as both test and data provider - consider refactoring.

test_all_views_have_buttons returns all_buttons (line 112), which is used by other tests. However, pytest ignores test return values. This dual-purpose design leads to the coupling issues noted above.

Consider extracting the view instantiation and button discovery logic into a pytest fixture that can be reused across tests.

src/switchcraft/gui_modern/views/settings_view.py (2)

703-709: Redundant close dialog wrapper.

The close_dlg function wraps _close and uses run_task if available, but since _show_dialog_with_flow is already called from the main thread (via run_task at line 792), the extra run_task check is unnecessary here.

---

1058-1068: Direct call with run_task fallback is reasonable but inconsistent with other patterns.

The comment states "Flet's update() is thread-safe" (line 1058), so calling _reload_app() directly is attempted first. However, other parts of this file (e.g., _start_github_login) use run_task as the primary path. Consider keeping the pattern consistent across the codebase for maintainability.

tests/test_language_change.py (1)

7-7: Unused import.

time is imported but not used in this file.

-import time

tests/test_button_functionality.py (3)

8-9: Unused imports.

inspect and importlib are imported but not used in this file.

-import inspect
-import importlib

---

42-49: Duplicated find_clickables helper function.

The same helper function is defined twice (lines 42-49 and 86-93). Extract it to module level or as a fixture to reduce duplication.

Extract to module level

+def find_clickables(control, clickables):
+    """Recursively find all controls with on_click handlers."""
+    if hasattr(control, 'on_click') and control.on_click is not None:
+        clickables.append(control)
+    if hasattr(control, 'content'):
+        find_clickables(control.content, clickables)
+    if hasattr(control, 'controls'):
+        for child in control.controls:
+            find_clickables(child, clickables)
+

 def test_home_view_buttons(mock_page):
     """Test that Home View buttons work."""
     from switchcraft.gui_modern.views.home_view import ModernHomeView

     navigate_calls = []
     def track_navigate(idx):
         navigate_calls.append(idx)

     view = ModernHomeView(mock_page, on_navigate=track_navigate)

-    # Find all clickable containers
-    def find_clickables(control, clickables):
-        if hasattr(control, 'on_click') and control.on_click is not None:
-            clickables.append(control)
-        if hasattr(control, 'content'):
-            find_clickables(control.content, clickables)
-        if hasattr(control, 'controls'):
-            for child in control.controls:
-                find_clickables(child, clickables)
-
     clickables = []
     find_clickables(view, clickables)

Also applies to: 86-93

---

175-175: Weak assertions: assert True provides no verification.

Both test_winget_view_search_button and test_intune_store_search_button end with assert True, which always passes and doesn't verify any behavior.

Consider verifying that the search was triggered or that the view state changed:

-        assert True  # Just verify view was created
+        # Verify view was created and search can be triggered
+        assert view is not None

Or remove the assertion entirely if the test's purpose is just to ensure no exceptions are raised during the flow.

Also applies to: 200-200

Configuration used: defaults

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

src/switchcraft/gui_modern/views/intune_store_view.py (1)

201-220: Align the UI-thread comment with the run_task path.

The comment says to avoid run_task, but the code now uses run_task as the primary marshalling mechanism. Update/remove the comment to avoid misleading future edits.

🛠️ Suggested fix

-            # Update UI on main thread - MUST use page.update() directly, not run_task
+            # Update UI on the page event loop; prefer run_task when available

docs/Intune_Configuration_Guide.md (1)

1-202: Optional: Consider adding a troubleshooting section.

The guide is comprehensive and well-structured. To further enhance it, consider adding:

• A troubleshooting section with additional common errors and solutions
• Guidance on validating that policies are applied correctly
• Links to related documentation (e.g., the INTUNE.md and PolicyDefinitions/README.md mentioned in the PR context)

These additions would make the guide even more valuable for administrators, but the current content is already solid and production-ready.

src/switchcraft/gui_modern/views/winget_view.py (1)

451-461: Consistent with the pattern above - same thread-safety concern applies.

The same fallback pattern is used here for error UI. Consider consolidating this into a helper method to reduce duplication.

♻️ Suggested helper to reduce duplication

def _run_on_ui_thread(self, func):
    """Execute function on UI thread with fallback."""
    try:
        func()
    except Exception as ex:
        logger.debug(f"Direct call failed: {ex}")
        if hasattr(self.app_page, 'run_task'):
            try:
                self.app_page.run_task(func)
            except Exception:
                pass

tests/test_winget_details.py (2)

70-78: Using time.sleep() for async synchronization is flaky.

Tests that rely on time.sleep() can fail intermittently on slow CI runners or pass despite bugs on fast machines. Consider using polling with a timeout or mocking the threading entirely.

♻️ Suggested improvement using polling

import time

def wait_for(condition, timeout=2.0, interval=0.05):
    """Wait for condition to be true, polling at interval."""
    start = time.time()
    while time.time() - start < timeout:
        if condition():
            return True
        time.sleep(interval)
    return False

# Usage in test:
assert wait_for(lambda: len(details_shown) > 0), "Details should be loaded"

---

174-196: Test verifies update calls but assertion at line 196 is weak.

The assertion assert len(update_calls) > 0 or len(page_update_calls) > 0 passes if either is called, but doesn't verify that updates happened at the right time or in the right sequence.

tests/test_all_buttons.py (1)

260-264: Swallowing handler exceptions may hide real bugs.

Printing failures but not failing the test means broken handlers go unnoticed. Consider collecting failures and asserting at the end, or using pytest.warns/pytest.raises selectively.

♻️ Suggested improvement to track failures

+        handler_failures = []
         for button in buttons:
             if hasattr(button, 'on_click') and button.on_click is not None:
                 try:
                     mock_event = MagicMock()
                     mock_event.control = button
                     mock_event.data = "true"
                     button.on_click(mock_event)
                 except Exception as e:
-                    print(f"Button handler failed in {view_name}: {e}")
+                    handler_failures.append(f"{view_name}: {e}")
 
         assert view is not None
+        # Optionally: assert not handler_failures, f"Handler failures: {handler_failures}"

tests/test_all_three_issues.py (1)

50-67: Mock AuthService fixture provides complete device flow mocking.

All necessary return values are configured for the GitHub device flow. However, there's redundancy: mock_service is created but only mock is returned.

♻️ Simplify fixture

 `@pytest.fixture`
 def mock_auth_service():
     """Mock AuthService responses."""
     with patch('switchcraft.gui_modern.views.settings_view.AuthService') as mock:
-        mock_service = MagicMock()
         mock.initiate_device_flow.return_value = {
             "device_code": "test_code",
             "user_code": "ABC-123",
             "verification_uri": "https://github.com/login/device",
             "interval": 5,
             "expires_in": 900
         }
         mock.poll_for_token.return_value = "test_token_123"
         mock.save_token = MagicMock()
-        mock_service.initiate_device_flow = mock.initiate_device_flow
-        mock_service.poll_for_token = mock.poll_for_token
-        mock_service.save_token = mock.save_token
-        return mock
+        yield mock

tests/test_ui_interactions_critical.py (1)

85-87: Avoid fixed sleeps in UI tests; poll for state instead.

Fixed waits add flakiness; polling for the dialog/open state (or goto_tab call) makes the tests more deterministic.

♻️ Suggested approach (apply similarly to the other sleep)

-    time.sleep(1.0)
+    for _ in range(20):
+        if mock_page.dialog and mock_page.dialog.open:
+            break
+        time.sleep(0.1)

Also applies to: 136-137

tests/test_github_login_real.py (1)

72-73: Replace fixed sleep with polling to reduce flakiness.

♻️ Suggested fix

-    time.sleep(0.5)
+    for _ in range(10):
+        if mock_page.dialog and mock_page.dialog.open:
+            break
+        time.sleep(0.05)

tests/test_button_functionality.py (2)

56-67: Don’t silently swallow all click failures.

Right now the tests can pass even if every handler crashes. Consider tracking successful clicks and asserting at least one succeeds (apply the same pattern to the other loops).

♻️ Example adjustment (apply similarly to other loops)

-    for clickable in clickables:
+    successful_clicks = 0
+    for clickable in clickables:
         if hasattr(clickable, 'on_click') and clickable.on_click:
             try:
                 mock_event = MagicMock()
                 mock_event.control = clickable
                 clickable.on_click(mock_event)
+                successful_clicks += 1
             except Exception as e:
                 # Some might fail due to missing dependencies, that's OK
                 pass
@@
-    assert len(clickables) > 0
+    assert len(clickables) > 0
+    assert successful_clicks > 0, "Expected at least one clickable handler to succeed"

Also applies to: 91-100, 117-127

---

167-168: Strengthen the “button works” assertions.

assert True / view is not None won’t catch regressions in button wiring. A minimal improvement is to assert the handler exists and is callable.

♻️ Suggested assertions

-        assert True  # Just verify view was created
+        assert search_button is not None, "Search button should exist"
+        assert callable(search_button.on_click), "Search button must have on_click handler"
@@
-        assert view is not None
+        assert view.btn_search is not None, "Search button should exist"
+        assert callable(view.btn_search.on_click), "Search button must have on_click handler"

Also applies to: 192-192

tests/test_intune_store_search.py (1)

150-216: Redundant imports and potential flakiness in timeout test.

1. Redundant imports: threading (line 153) and os (line 154) are already imported at module level.

2. Potential flakiness: The assertion search_started.wait(timeout=1.0) at line 205 may fail intermittently. The search_started.set() is called inside slow_search, but slow_search is assigned to mock_intune_service.search_apps (line 170). The actual _run_search method may call search_apps or list_apps depending on the query value. Since list_apps is not mocked with this behavior in this test, if the implementation path changes, the test could break.

3. Improved from past review: The test now has meaningful assertions (lines 211-215) instead of assert True.

Remove redundant imports

 def test_intune_search_timeout_mechanism(mock_page, mock_intune_service):
     """Test that Intune search properly times out after 60 seconds."""
     from switchcraft.gui_modern.views.intune_store_view import ModernIntuneStoreView
-    import threading
-    import os
 
     # Skip in CI to avoid 65 second wait

Configuration used: defaults

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

.github/workflows/ci.yml (2)

28-43: LGTM! Previous review concerns addressed correctly.

The job-level if referencing matrix has been removed, and conditions are now properly placed at the step level where matrix context is available. The allow-prereleases flag correctly enables Python 3.15 prerelease support.

---

46-93: Same DRY opportunity as in test.yml.

The version-gating condition is repeated across all steps. Consider extracting it to reduce duplication, as noted for test.yml.

tests/test_ui_interactions_critical.py (1)

200-219: Dropdown lookup should be recursive.

This test only checks direct children, so it can miss a nested language dropdown. Use a recursive search like the one in your other test.

🔧 Suggested fix

-    lang_dd = None
-    for control in general_tab.controls:
-        if isinstance(control, ft.Dropdown):
-            lang_dd = control
-            break
+    def find_dropdown(control):
+        if isinstance(control, ft.Dropdown):
+            return control
+        if hasattr(control, 'controls'):
+            for child in control.controls:
+                result = find_dropdown(child)
+                if result:
+                    return result
+        if hasattr(control, 'content'):
+            return find_dropdown(control.content)
+        return None
+    lang_dd = find_dropdown(general_tab)

.github/workflows/test.yml (1)

33-49: Consider extracting the repeated condition into a reusable pattern.

The same condition (matrix.python-version == '3.14' || (matrix.python-version == '3.15' && github.event_name == 'push' && github.ref == 'refs/heads/main')) is repeated across multiple steps. While this works, you could consider using a job output or env variable to reduce duplication.

♻️ Optional: Use an env variable for the condition

    env:
      SHOULD_RUN: ${{ matrix.python-version == '3.14' || (matrix.python-version == '3.15' && github.event_name == 'push' && github.ref == 'refs/heads/main') }}
    steps:
      # ...
      - name: Install dependencies
        if: env.SHOULD_RUN == 'true'
        run: |
          # ...

src/switchcraft/gui_modern/views/intune_store_view.py (1)

188-193: Redundant import: time is already imported at file top.

The import time on line 189 is unnecessary since time is already imported at line 4.

🧹 Remove redundant import

-            # Wait a bit more to ensure result_holder is set (max 1 second)
-            import time
+            # Wait a bit more to ensure result_holder is set (max 1 second)
             timeout_count = 0

scripts/build_release.ps1 (1)

155-173: Prefer an env override for the fallback version.

Hard‑coding the fallback version can drift from release tooling. Consider reading an env override first and derive the numeric variant from it.

♻️ Suggested tweak

-# Fallback version if extraction fails (should match current release cycle)
-$AppVersion = "2026.1.2"
-$AppVersionNumeric = "2026.1.2"
+# Fallback version if extraction fails (should match current release cycle)
+$FallbackVersion = if ($env:FALLBACK_VERSION) { $env:FALLBACK_VERSION } else { "2026.1.2" }
+$AppVersion = $FallbackVersion
+$AppVersionNumeric = $FallbackVersion -replace '-.*', ''

tests/test_intune_store_search.py (1)

64-107: Avoid real 65–70s sleeps in background threads.

Even with timeout simulation, the daemon thread still sleeps for a long time. Consider using a synchronization primitive (e.g., an event wait) or a stub that blocks without long sleeps to keep tests fast and avoid lingering threads.

Also applies to: 177-251

src/switchcraft/gui_modern/views/winget_view.py (1)

243-245: Route _show_list through _run_ui_update for thread safety.
_search() runs in a background thread; if Flet requires UI updates on the main thread, this call should be marshaled.

♻️ Suggested change

-                data = result_holder["data"] if result_holder["data"] is not None else []
-                self._show_list(data, filter_by, query)
+                data = result_holder["data"] if result_holder["data"] is not None else []
+                self._run_ui_update(lambda: self._show_list(data, filter_by, query))

Please confirm Flet’s guidance on UI updates from background threads to ensure this aligns with best practices.

.github/scripts/generate_release_version.py (1)

83-86: Consider anchoring the validation regex for consistency.

The regex r'^\d+\.\d+\.\d+' only checks the prefix, so it would accept malformed versions like 1.2.3.4.5 or 1.2.3foo. The workflow file uses a stricter anchored pattern. For consistency:

♻️ Suggested stricter validation

-        if not next_version or not re.match(r'^\d+\.\d+\.\d+', next_version):
+        if not next_version or not re.match(r'^\d+\.\d+\.\d+(\.[a-z]+\d+|[a-z]+\d+)?(\+[a-zA-Z0-9.-]+)?$', next_version):

pyproject.toml (1)

17-32: Duplicate requests dependency.

The requests package appears twice in the dependencies list (lines 20 and 31). While this won't cause runtime issues, it should be cleaned up.

♻️ Remove duplicate

     "defusedxml",
     "PyJWT",
-    "winotify",
-    "requests"
+    "winotify"
 ]

src/switchcraft/services/intune_service.py (1)

487-489: Creating a new RequestException loses call site information.

Line 489 creates a new RequestException with a modified message, which discards the original exception's type-specific information (e.g., ConnectionError, HTTPError). Unlike the Timeout handling above it, this approach loses specificity. Consider using bare raise for consistency with list_apps.

🔧 Suggested fix

         except requests.exceptions.RequestException as e:
             logger.error(f"Network error getting app details: {e}")
-            raise requests.exceptions.RequestException(f"Network error: {str(e)}") from e
+            raise

tests/test_all_three_issues.py (1)

66-137: Optional: replace fixed sleeps with polling to reduce flakiness.
The tests already use async flows; a small poll_until helper would make timing more robust.

tests/test_github_login_real.py (1)

32-55: Pipeline failure: mock_page fixture missing NavigationDrawer handling.

The pipeline fails at line 142 because mock_page.end_drawer is never set. The mock_open function only handles ft.AlertDialog but not ft.NavigationDrawer. This was flagged in a previous review but appears not fully addressed in this file (though it was fixed in test_ui_interactions_critical.py).

🐛 Fix to handle NavigationDrawer

 `@pytest.fixture`
 def mock_page():
     """Create a mock Flet page."""
     page = MagicMock(spec=ft.Page)
     page.dialog = None
+    page.end_drawer = None
     page.update = MagicMock()
     page.snack_bar = MagicMock(spec=ft.SnackBar)
     page.snack_bar.open = False
     page.switchcraft_app = MagicMock()

     # Mock run_task to actually execute the function
     def run_task(func):
         func()
     page.run_task = run_task

     # Mock page.open to set dialog and open it
     def mock_open(control):
         if isinstance(control, ft.AlertDialog):
             page.dialog = control
             control.open = True
+        elif isinstance(control, ft.NavigationDrawer):
+            page.end_drawer = control
+            control.open = True
         page.update()
     page.open = mock_open

     return page

tests/test_settings_language.py (1)

31-36: Consider setting run_task in setUp for consistency.

The run_task mock is set after ModernSettingsView initialization. While this works here since _on_lang_change is the method under test, other test files (e.g., test_ui_interactions_critical.py, test_github_login_real.py) set run_task in the fixture/setUp before view creation. For consistency and to avoid potential issues if the view's __init__ ever uses run_task, consider moving this to setUp.

Suggested change

     def setUp(self):
         ...
         self.page.show_snack_bar = MagicMock()
+        # Ensure run_task is available for UI updates
+        self.page.run_task = lambda func: func()

     `@patch`('switchcraft.utils.config.SwitchCraftConfig.set_user_preference')
     `@patch`('switchcraft.utils.i18n.i18n.set_language')
     def test_language_change_immediate(self, mock_set_language, mock_set_pref):
         """Test that language change is applied immediately."""
         from switchcraft.gui_modern.views.settings_view import ModernSettingsView

         view = ModernSettingsView(self.page)
-        # Ensure run_task is available for UI updates
-        self.page.run_task = lambda func: func()

         # Simulate language change
         view._on_lang_change("en")

tests/test_ui_interactions_critical.py (2)

15-33: poll_until helper is duplicated across test files.

This utility function is duplicated verbatim in test_github_login_real.py. Consider extracting it to a shared location like tests/conftest.py or a tests/helpers.py module.

---

35-68: mock_page fixture is duplicated across test files.

This fixture is nearly identical to the one in test_github_login_real.py. Consider moving shared fixtures to tests/conftest.py for reuse across test modules.

tests/test_github_login_real.py (1)

82-104: Verify mock_page supports NavigationDrawer/end_drawer.

This test assumes page.open(...) sets end_drawer when a NavigationDrawer is opened. Please confirm the shared mock_page fixture in tests/conftest.py implements that; otherwise these assertions will fail.

#!/bin/bash
# Inspect mock_page fixture for NavigationDrawer/end_drawer handling
fd -t f 'conftest.py'
rg -n "def mock_page" -C3 $(fd -t f 'conftest.py')
rg -n "end_drawer|NavigationDrawer|page\.open" -C3 $(fd -t f 'conftest.py')

tests/conftest.py (1)

95-104: Consider handling SnackBar in mock_page.open.

Your UI code uses page.open(page.snack_bar). The mock currently ignores SnackBar, so tests that assert snack-bar state can’t observe it.

♻️ Suggested tweak

             def mock_open(control):
                 if isinstance(control, ft.AlertDialog):
                     self.dialog = control
                     control.open = True
                 elif isinstance(control, ft.NavigationDrawer):
                     self.end_drawer = control
                     control.open = True
+                elif isinstance(control, ft.SnackBar):
+                    self.snack_bar = control
+                    control.open = True
                 self.update()

tests/test_template_company.py (1)

5-13: Align fallback CI detection with shared helper.

To avoid drift, mirror the conftest.is_ci_environment() conditions in the fallback.

♻️ Suggested tweak

     def is_ci_environment():
-        return os.environ.get('CI') == 'true' or os.environ.get('GITHUB_ACTIONS') == 'true'
+        return (
+            os.environ.get('CI') == 'true' or
+            os.environ.get('GITHUB_ACTIONS') == 'true' or
+            os.environ.get('GITHUB_RUN_ID') is not None
+        )

tests/test_github_login.py (1)

23-45: Consider reusing the shared mock_page fixture.

There’s already a richer mock_page in tests/conftest.py (end_drawer + open semantics). Reusing it avoids drift across UI tests.

♻️ Suggested refactor

-@pytest.fixture
-def mock_page():
-    """Create a mock Flet page with run_task support."""
-    page = MagicMock(spec=ft.Page)
-    page.dialog = None
-    page.update = MagicMock()
-    page.snack_bar = MagicMock(spec=ft.SnackBar)
-    page.snack_bar.open = False
-
-    # Mock run_task to actually execute the function
-    def run_task(func):
-        func()
-    page.run_task = run_task
-
-    # Mock page.open to set dialog and open it
-    def mock_open(control):
-        if isinstance(control, ft.AlertDialog):
-            page.dialog = control
-            control.open = True
-        page.update()
-    page.open = mock_open
-
-    return page
+from tests.conftest import mock_page

tests/test_intune_store_search.py (1)

24-55: Consider reusing shared poll_until/mock_page from tests.conftest.

This avoids helper drift and keeps test behavior consistent across modules.

tests/test_winget_details.py (1)

23-55: Consider reusing shared poll_until/mock_page from tests.conftest.

This keeps helper behavior consistent across the test suite.

tests/test_winget_details.py (1)

12-39: Consolidate fallback helpers to avoid drift.

The inline fallback duplicates logic from tests.conftest. Consider moving the fallback into a shared helper (or failing fast with a clearer message) so behavior doesn’t silently diverge between environments.

src/switchcraft/gui_modern/views/settings_view.py (1)

808-821: Same run_task concern: lambda wrapper returns sync result.

The lambda: _show_dialog_with_flow(flow) wrapper creates a synchronous callable, which may not integrate correctly with run_task that expects async callables. The extensive fallback handling is good for resilience, but consider consolidating the async/sync approach.

src/switchcraft/gui_modern/app.py (1)

1825-1826: Redundant assignment of page.switchcraft_app.

This assignment duplicates line 43 in __init__ where self.page.switchcraft_app = self is already set. Since main() creates a ModernApp which runs __init__, the attribute is already set by the time line 1826 executes.

The duplication is harmless but could be removed for clarity.

♻️ Optional: Remove redundant assignment

 def main(page: ft.Page):
     # Add restart method to page for injection if needed, or pass app instance.
     app = ModernApp(page)
     page._show_restart_countdown = app._show_restart_countdown
-    # Make app accessible to views via page
-    page.switchcraft_app = app

src/switchcraft/gui_modern/views/group_manager_view.py (1)

383-389: Remove unused instance attribute assignment.

self.dlg_add_member = add_dlg on line 389 is assigned but never referenced elsewhere in the codebase. The local add_dlg variable is correctly used in the closure-based callbacks, making the instance attribute redundant.

♻️ Suggested fix: remove unused assignment

             # Create dialog first so it can be referenced in nested functions
             add_dlg = ft.AlertDialog(
                 title=ft.Text(i18n.get("dlg_add_member") or "Add Member"),
                 content=ft.Column([search_box, results_list], height=300, width=400),
                 actions=[ft.TextButton(i18n.get("btn_close") or "Close", on_click=lambda e: self._close_dialog(add_dlg))]
             )
-            self.dlg_add_member = add_dlg

src/switchcraft/gui_modern/views/settings_view.py (1)

1058-1106: Consistent pattern with run_task fallbacks, but same async/sync concern applies.

The _reload_app() function follows the same pattern as the GitHub login flow with run_task and fallbacks. The same async/sync concern noted earlier applies here—_reload_app is synchronous but passed to run_task.

The extensive fallback handling ensures users always get feedback, which is good for resilience.

Consider defining a shared helper for this run_task-with-fallback pattern to reduce code duplication across _start_github_login and _on_lang_change:

Example helper pattern

def _run_on_main_thread(self, func):
    """Run a function on the main thread with fallback."""
    if hasattr(self.app_page, 'run_task'):
        try:
            # If func is sync, wrap it in async
            async def _wrapper():
                func()
            self.app_page.run_task(_wrapper)
            return
        except Exception as ex:
            logger.exception(f"run_task failed: {ex}")
    # Fallback: direct call
    try:
        func()
    except Exception as ex:
        logger.exception(f"Direct call failed: {ex}")

tests/test_winget_details.py (1)

117-135: Make the loading-state assertion deterministic (avoid timing flakiness).

The time.sleep(0.3) approach is fragile across different environments. This was flagged in a previous review.

tests/test_template_company.py (1)

5-17: Consider using relative import for conftest.

The import from conftest import is_ci_environment relies on pytest's special handling of conftest.py. When running tests directly (e.g., python tests/test_template_company.py), this import may fail since conftest isn't a standard module on the path. The fallback handles this gracefully.

Minor style note: stdlib imports (os) conventionally go before local/project imports per PEP 8.

Suggested improvement

 import unittest
+import os
 from pathlib import Path
 from unittest.mock import patch
 from switchcraft.utils.templates import TemplateGenerator
-import os

 # Import CI detection helper
 try:
-    from conftest import is_ci_environment
+    from tests.conftest import is_ci_environment
 except ImportError:
     # Fallback if conftest not available - mirror conftest.is_ci_environment() conditions
     def is_ci_environment():
         return (
             os.environ.get('CI') == 'true' or
             os.environ.get('GITHUB_ACTIONS') == 'true' or
             os.environ.get('GITHUB_RUN_ID') is not None
         )

src/switchcraft/gui_modern/app.py (1)

279-304: Multiple page.update() calls may cause UI flicker.

There are 5 page.update() calls in quick succession (lines 283, 287, 293, 301, 304). While defensive, this could cause noticeable flicker. Consider consolidating updates or using a single update after all state changes are applied.

♻️ Suggested consolidation

             # Set drawer on page FIRST
             self.page.end_drawer = drawer
-
-            # Force update to attach drawer to page
-            self.page.update()
-
-            # Now set open BEFORE updating (Flet needs this order)
+            # Set open state
             drawer.open = True
-            self.page.update()

             # Try additional methods if drawer didn't open
             try:
                 if hasattr(self.page, 'open'):
                     self.page.open(drawer)
-                    self.page.update()
             except Exception as ex:
                 logger.debug(f"page.open() not available or failed: {ex}, using direct assignment")

             # Final verification and update
             if not drawer.open:
                 logger.warning("Drawer open flag is False, forcing it to True")
                 drawer.open = True
-                self.page.update()

-            # Final update to ensure drawer is visible
+            # Single update after all state changes
             self.page.update()

tests/test_winget_details.py (1)

161-193: Refactor duplicate collect_text function.

The collect_text helper is defined twice within the same test (lines 165-172 for the polling condition and lines 183-190 for the final assertion). Extract this to a single nested function or use a closure to avoid duplication.

♻️ Suggested refactor

+    def collect_text(control, error_texts):
+        if isinstance(control, ft.Text):
+            error_texts.append(control.value)
+        elif hasattr(control, 'controls'):
+            for c in control.controls:
+                collect_text(c, error_texts)
+        elif hasattr(control, 'content'):
+            collect_text(control.content, error_texts)
+
     # Wait for error handling using polling
     def error_shown():
         if view.details_area is None:
             return False
         error_texts = []
-        def collect_text(control):
-            if isinstance(control, ft.Text):
-                error_texts.append(control.value)
-            elif hasattr(control, 'controls'):
-                for c in control.controls:
-                    collect_text(c)
-            elif hasattr(control, 'content'):
-                collect_text(control.content)
-        collect_text(view.details_area)
+        collect_text(view.details_area, error_texts)
         return any("error" in str(text).lower() or "failed" in str(text).lower() for text in error_texts)

     assert poll_until(error_shown, timeout=2.0), "Error should be shown within timeout"

     # Check that error was shown
     assert view.details_area is not None
     assert isinstance(view.details_area, ft.Column)
     # Should have error message
     error_texts = []
-    def collect_text(control):
-        if isinstance(control, ft.Text):
-            error_texts.append(control.value)
-        elif hasattr(control, 'controls'):
-            for c in control.controls:
-                collect_text(c)
-        elif hasattr(control, 'content'):
-            collect_text(control.content)
-
-    collect_text(view.details_area)
+    collect_text(view.details_area, error_texts)

src/switchcraft/gui_modern/views/settings_view.py (1)

746-782: Redundant dialog open check.

Lines 777-781 force dlg.open = True if the flag is False, but this happens immediately after attempting to open the dialog. If page.open() or manual assignment succeeded, the flag should already be True. This suggests a potential race or the dialog opening is unreliable.

Consider adding a small delay or verifying why the dialog might not be open after the display attempts, rather than forcing the flag.

tests/test_all_buttons.py (2)

63-140: Consider not storing view instances in the returned dictionary.

The collect_all_buttons function stores the entire view instance in the returned dictionary (line 122: 'view': view). This could lead to memory issues if views are large or hold references to resources. Since the view isn't used after button collection, consider removing it.

♻️ Suggested change

                 all_buttons[view_class.__name__] = {
                     'buttons': buttons,
                     'count': len(buttons),
-                    'view': view
                 }

---

230-241: Consider extracting mock_page setup to a helper or fixture.

The mock_page setup code is duplicated between collect_all_buttons (lines 74-86) and test_view_buttons_work (lines 230-241). Extract this to a shared fixture or helper function.

♻️ Suggested refactor

def create_mock_page():
    """Create a mock Flet page for testing."""
    mock_page = MagicMock(spec=ft.Page)
    mock_page.update = MagicMock()
    mock_page.switchcraft_app = MagicMock()
    mock_page.switchcraft_app.goto_tab = MagicMock()
    mock_page.switchcraft_app._current_tab_index = 0
    mock_page.dialog = None
    mock_page.end_drawer = None
    mock_page.snack_bar = None
    mock_page.open = MagicMock()
    mock_page.close = MagicMock()
    mock_page.run_task = lambda func: func()
    type(mock_page).page = property(lambda self: mock_page)
    return mock_page

Then use mock_page = create_mock_page() in both places.

Also applies to: 74-86